Making _set_seed abstract
#770
Conversation
facebook-github-bot
added
the
CLA Signed
Codecov Report
@@ Coverage Diff @@
## main #770 +/- ##
==========================================
- Coverage 88.75% 88.73% -0.02%
==========================================
Files 123 123
Lines 20987 20986 -1
==========================================
- Hits 18626 18622 -4
- Misses 2361 2364 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
vmoens
left a comment
vmoens
left a comment
There was a problem hiding this comment.
LGTM! Let's merge main in this branch to resolve the conflicts
| def set_seed( | ||
| self, seed: Optional[int] = None, static_seed: bool = False | ||
| ) -> Optional[int]: | ||
| # This method is not used in transformed environments |
There was a problem hiding this comment.
Is this true? The message matches the one from _set_seed
There was a problem hiding this comment.
Maybe we could use the docstring above?
Another note: I don't remember why we're not using _set_seed to be honest. I guess it's because the seed iteration would occur twice if transformed_env._set_seed would call ˋbase_env.set_seed, but what if transformed_env._set_seed would call ˋbase_env._set_seed?
There was a problem hiding this comment.
Is this true? The message matches the one from _set_seed
Oh sorry it was not meant to be present in set_seed. Fixed
Maybe we could use the docstring above?
Yes, fixed
Another note: I don't remember why we're not using _set_seed to be honest. I guess it's because the seed iteration would occur twice if
transformed_env._set_seedwould call ˋbase_env.set_seed, but what iftransformed_env._set_seedwould call ˋbase_env._set_seed?
Basically _set_seed is only called in the original implementation of set_seed in BaseEnv. Given that TransformedEnv and BatchedEnv override set_seed (this would not normally be the case for inheritors of BaseEnv and it is the case just for these two examples as they are wrappers which need to change the seeding logic) the inner _set_seed is not called anymore and we just implement it with a pass (whatever we implement it with doesn't matter as it is never called)
torchrl/envs/vec_env.py
Outdated
|
|
||
| def _set_seed(self, seed: Optional[int]): | ||
| # This method is not used in batched envs | ||
| return |
There was a problem hiding this comment.
Maybe pass instead of return? Seems semantically more appropriate to me
|
All the comments should have been addressed |
3 participants
Description
Making
_set_seedabstractMotivation and Context
In the current implementation of
EnvBasethere is a convention for certain methods to have and abstract counterpart.step()->_step()reset()->_reset()The same seems to be valid for
set_seed(), except that_set_seed()is not officially abstract.Following the convention it would be intuitive that inheritors of
EnvBasewould override_set_seed(), but there are currenty some inheritors which do not implement_set_seed(). This breaks the intuitive convention.In this PR we made
_set_seed()abstract. This caused some inheritors which overrideset_seed()(like_BatchedEnvandTransformedEnv) to need to implement_set_seed()with no action (as these classes act as wrappers).Types of changes
What types of changes does your code introduce? Remove all that do not apply:
Checklist
Go over all the following points, and put an
xin all the boxes that apply.If you are unsure about any of these, don't hesitate to ask. We are here to help!
I have checked that the tests in test_env.py which I am able to run (without external libraries and cuda) are all passing and linted.